add skip_prompt #4992
add skip_prompt #4992
Conversation
WalkthroughThe changes introduce a new optional parameter, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/executor.py (1)
Line range hint
747-757: Implementation incomplete:skip_promptparameter is not being used.While the
skip_promptparameter has been added to the method signature and documented, it's not being passed through to the underlying operation. The parameter needs to be included in the params dictionary that's passed to the trigger call.Apply this diff to include the parameter:
def prompt( self, operator_uri, params=None, on_success=None, on_error=None, skip_prompt=False, ): return self.trigger( "prompt_user_for_operation", params=_convert_callables_to_operator_uris( { "operator_uri": operator_uri, "panel_id": self.panel_id, "params": params, "on_success": on_success, "on_error": on_error, + "skip_prompt": skip_prompt, } ), )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/operators/src/built-in-operators.ts (2 hunks)
- fiftyone/operators/executor.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/operators/src/built-in-operators.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
| const shouldPrompt = !params.skip_prompt; | ||
|
|
||
| triggerEvent(panelId, { | ||
| operator: operator_uri, | ||
| params, | ||
| prompt: true, | ||
| prompt: shouldPrompt, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve parameter access consistency and simplify logic.
The skip_prompt parameter is accessed differently from other parameters and the logic can be simplified.
Apply this diff to improve the code:
- const shouldPrompt = !params.skip_prompt;
+ const { operator_uri, on_success, on_error, skip_prompt } = params;
+ const shouldPrompt = !skip_prompt;
triggerEvent(panelId, {
operator: operator_uri,
params,
- prompt: shouldPrompt,
+ prompt: !skip_prompt,
callback: (result: OperatorResult) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shouldPrompt = !params.skip_prompt; | |
| triggerEvent(panelId, { | |
| operator: operator_uri, | |
| params, | |
| prompt: true, | |
| prompt: shouldPrompt, | |
| const { operator_uri, on_success, on_error, skip_prompt } = params; | |
| const shouldPrompt = !skip_prompt; | |
| triggerEvent(panelId, { | |
| operator: operator_uri, | |
| params, | |
| prompt: !skip_prompt, |
| @@ -1027,6 +1027,7 @@ | |||
| inputs.obj("params", { label: "Params" }); | |||
| inputs.str("on_success", { label: "On success" }); | |||
| inputs.str("on_error", { label: "On error" }); | |||
| inputs.str("skip_prompt", { label: "Skip prompt", default: false }); | |||
There was a problem hiding this comment.
Fix type mismatch for skip_prompt parameter.
The skip_prompt parameter is defined as a string but used as a boolean in the execute method. This could lead to unexpected behavior.
Apply this diff to fix the type and improve the description:
- inputs.str("skip_prompt", { label: "Skip prompt", default: false });
+ inputs.bool("skip_prompt", {
+ label: "Skip prompt",
+ description: "When true, bypasses the user prompt and executes immediately with provided parameters",
+ default: false
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inputs.str("skip_prompt", { label: "Skip prompt", default: false }); | |
| inputs.bool("skip_prompt", { | |
| label: "Skip prompt", | |
| description: "When true, bypasses the user prompt and executes immediately with provided parameters", | |
| default: false | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/packages/operators/src/built-in-operators.ts (1)
1030-1030: Add a description for the skip_prompt parameter.Consider adding a description to clarify the purpose and impact of skipping the prompt.
- inputs.bool("skip_prompt", { label: "Skip prompt", default: false }); + inputs.bool("skip_prompt", { + label: "Skip prompt", + description: "When true, bypasses the user prompt and executes immediately with the provided parameters", + default: false + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/operators/src/built-in-operators.ts (2 hunks)
- fiftyone/operators/executor.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fiftyone/operators/executor.py
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/operators/src/built-in-operators.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/operators/src/built-in-operators.ts (2)
1041-1046: Reuse existing review comment about parameter access consistency.
1049-1061: LGTM! Error handling and success callbacks are well implemented.The implementation properly handles both error and success cases, with appropriate type checking and event triggering.
|
Working as of latest |
Allows disabling of prompt when using
ctx.prompt(), which will execute the operator immediately given the params.Summary by CodeRabbit
New Features
skip_promptoption for thePromptUserForOperationclass, allowing users to bypass prompts during operation execution.skip_promptparameter to thepromptmethod in theExecutionContextclass for enhanced control over user prompts.Documentation
skip_promptparameter, improving clarity on its usage.